Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve memory diagnoser accuracy #2562

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timcassell
Copy link
Collaborator

@timcassell timcassell commented Apr 15, 2024

This doesn't fix the zero-alloc measurement, but it does fix a bug from the threading diagnoser interfering with the results. The other changes give us the highest confidence that any errant measurements are not our fault. The rest is up to the runtime.

Fixes #1542

@timcassell
Copy link
Collaborator Author

It looks like I may have resolved the zero-alloc issue!

New results (using the same benchmark from #1543):

Method Runtime Allocated
Benchmark1 .NET 6.0 2 B
Benchmark2 .NET 6.0 2 B
Benchmark3 .NET 6.0 2 B
Benchmark4 .NET 6.0 2 B
Benchmark5 .NET 6.0 2 B
Benchmark1 .NET 7.0 -
Benchmark2 .NET 7.0 -
Benchmark3 .NET 7.0 -
Benchmark4 .NET 7.0 -
Benchmark5 .NET 7.0 -
Benchmark1 .NET 8.0 -
Benchmark2 .NET 8.0 -
Benchmark3 .NET 8.0 -
Benchmark4 .NET 8.0 -
Benchmark5 .NET 8.0 -

The resolution ended up being moving the GcStats measurement to its own method, and removing GC.Collect() from the allocation measurement. Doing only 1 of those still wasn't getting the expected results.

I was unable to repro the issue in a plain console application, so I'm not sure why GC.Collect() interferes with the results (I would have reported a bug in the runtime repo if I could).

PTAL @adamsitnik

@timcassell timcassell changed the title Try to stabilize memory diagnoser Fix memory diagnoser Apr 15, 2024
@timcassell
Copy link
Collaborator Author

I added the thread sleep from #1543 in Core runtimes 3.0 to 6.0, and now I'm getting 0 measurements across the board:

Method Runtime Allocated
Benchmark1 .NET 6.0 -
Benchmark2 .NET 6.0 -
Benchmark3 .NET 6.0 -
Benchmark4 .NET 6.0 -
Benchmark5 .NET 6.0 -
Benchmark1 .NET 7.0 -
Benchmark2 .NET 7.0 -
Benchmark3 .NET 7.0 -
Benchmark4 .NET 7.0 -
Benchmark5 .NET 7.0 -
Benchmark1 .NET 8.0 -
Benchmark2 .NET 8.0 -
Benchmark3 .NET 8.0 -
Benchmark4 .NET 8.0 -
Benchmark5 .NET 8.0 -
Benchmark1 .NET Core 2.1 -
Benchmark2 .NET Core 2.1 -
Benchmark3 .NET Core 2.1 -
Benchmark4 .NET Core 2.1 -
Benchmark5 .NET Core 2.1 -
Benchmark1 .NET Core 3.0 -
Benchmark2 .NET Core 3.0 -
Benchmark3 .NET Core 3.0 -
Benchmark4 .NET Core 3.0 -
Benchmark5 .NET Core 3.0 -

@timcassell timcassell force-pushed the stabilize-memorydiagnoser branch 3 times, most recently from 0624a8c to 917988c Compare April 23, 2024 00:53
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big thanks for working on improving the accuracy of the memory diagnoser @timcassell !

Let's wait for the response from @Maoni0.

src/BenchmarkDotNet/Engines/GcStats.cs Outdated Show resolved Hide resolved
src/BenchmarkDotNet/Engines/GcStats.cs Outdated Show resolved Hide resolved
src/BenchmarkDotNet/Engines/Engine.cs Outdated Show resolved Hide resolved
@timcassell

This comment was marked as outdated.

@timcassell

This comment was marked as outdated.

@timcassell
Copy link
Collaborator Author

Update: I found the culprit and opened an issue in dotnet/runtime. dotnet/runtime#101536

@timcassell
Copy link
Collaborator Author

I disabled the flaky tests and left a todo to re-enable them when that issue is fixed.

This is as accurate as we can make the memory diagnoser from our side. I think this should be good to merge now, unless you have any further feedback @adamsitnik.

@timcassell timcassell force-pushed the stabilize-memorydiagnoser branch 2 times, most recently from 060ce34 to 63b62a9 Compare April 25, 2024 16:06
@timcassell
Copy link
Collaborator Author

Ok, I re-enabled the tests and added a finalizer thread blocker to make the tests less flaky (nice idea @jkotas). This does not have any effect on real world measurements, though.

@timcassell
Copy link
Collaborator Author

Wow, even with tiered jit disabled and finalizer thread blocked, the CI is still flaky.

2024-04-28T00:50:55.0136465Z   Failed AllocationQuantumIsNotAnIssueForNetCore21Plus(toolchain: .NET 8.0) [9 s]
2024-04-28T00:50:55.0137358Z   Error Message:
2024-04-28T00:50:55.0137773Z    Assert.Equal() Failure: Values differ
2024-04-28T00:50:55.0138293Z Expected: 88
2024-04-28T00:50:55.0138634Z Actual:   176

I don't know what causes that behavior, but I just moved the MemoryDiagnoserTests to ManualRunning to stabilize the CI.

@jkotas
Copy link
Member

jkotas commented Apr 28, 2024

Wow, even with tiered jit disabled and finalizer thread blocked, the CI is still flaky.

My guess is that the next problem are the event source background threads.

@timcassell
Copy link
Collaborator Author

My guess is that the next problem are the event source background threads.

Can you provide more details? I couldn't find information about it from google.

@jkotas
Copy link
Member

jkotas commented Apr 28, 2024

EventSource and EventPipe create background threads that may allocate. These background threads should be only doing work if there is something listening to the events. It turns out that there is pretty much always something listening to .NET runtime events in typical machine configs these days.

@timcassell
Copy link
Collaborator Author

You mentioned disabling that in the other issue. How can we do that?

@jkotas
Copy link
Member

jkotas commented Apr 28, 2024

Try to add this to your .csproj:

  <ItemGroup>
    <RuntimeHostConfigurationOption Include="System.Diagnostics.Tracing.EventSource.IsSupported" Value="false" />
  </ItemGroup>

@timcassell
Copy link
Collaborator Author

Well, that didn't seem to help. The test failed exactly the same.

@jkotas
Copy link
Member

jkotas commented Apr 28, 2024

Does the test harness create any child processes to run the actual tests? If yes, does this setting propagate to the child processes?

@timcassell
Copy link
Collaborator Author

timcassell commented Apr 28, 2024

Does the test harness create any child processes to run the actual tests? If yes, does this setting propagate to the child processes?

Yes and yes, RuntimeHostConfigurationOption is one of the settings we explicitly copy.

I also verified it by adding this before the allocation measurement

var switchExists = AppContext.TryGetSwitch("System.Diagnostics.Tracing.EventSource.IsSupported", out var isEnabled);
Host.WriteLine($"// EventSource switchExists: {switchExists}, isEnabled: {isEnabled}");

and it printed // EventSource switchExists: True, isEnabled: False.

@timcassell timcassell changed the title Fix memory diagnoser Improve memory diagnoser accuracy May 1, 2024
@timcassell
Copy link
Collaborator Author

@adamsitnik Do you have any more feedback here?

…cations.

Warm up allocation measurement before taking actual measurement.
Isolated allocation measurement.
Changed some `RuntimeInformation` properties to static readonly fields.
Removed enable monitoring in Engine (GcStats handles it).
Removed `GC.Collect()` from allocation measurement.
Sleep thread to account for tiered jit in Core runtimes 3.0 to 6.0.
Updated MemoryDiagnoserTests.
Block finalizer thread during memory tests.
Disabled EventSource for integration tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants